refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators by zackees · Pull Request #1294 · FastLED/fbuild

zackees · 2026-08-10T00:19:54Z

Summary

Rolls out ensure_lib_deps() to all 11 remaining build orchestrators (fixes #1292), then refactors the duplicated inline block into a single pipeline::resolve_lib_deps() function.

Before

Each orchestrator had a ~55-line copy-paste block:

let lib_deps = ctx.config.get_lib_deps(&params.env_name)?;
let lib_ignore = ctx.config.get_lib_ignore(&params.env_name).unwrap_or_default();
let lib_archives: Vec<PathBuf>;
if !lib_deps.is_empty() {
    let temp_compiler = XxxCompiler::new(...);
    let c_flags_temp = temp_compiler.c_flags();  // or Compiler::c_flags(&temp)
    let cpp_flags_temp = temp_compiler.cpp_flags();
    let dep_ar_path = toolchain.get_ar_path();
    let dep_gcc_ar_path = toolchain.get_gcc_ar_path();
    let dep_lib_ar_path = pipeline::pick_archiver(...);
    let libs_dir = ctx.build_dir.join("libs");  // or build_dir.join("libs")
    let (lib_include_dirs, archives) = pipeline::ensure_lib_deps(...).await?;
    include_dirs.extend(lib_include_dirs);
    lib_archives = archives;
} else {
    lib_archives = Vec::new();
}

After

The invariant body is extracted into pipeline::resolve_lib_deps():

let lib_deps = ctx.config.get_lib_deps(&params.env_name)?;
let lib_ignore = ctx.config.get_lib_ignore(&params.env_name).unwrap_or_default();
let lib_archives = if !lib_deps.is_empty() {
    let temp_compiler = XxxCompiler::new(...);
    pipeline::resolve_lib_deps(
        &lib_deps, &lib_ignore,
        &params.project_dir, &ctx.build_dir,
        &toolchain.get_gcc_path(), &toolchain.get_gxx_path(),
        &toolchain.get_ar_path(), &toolchain.get_gcc_ar_path(),
        &temp_compiler.c_flags(), &temp_compiler.cpp_flags(),
        &mut include_dirs,
        params.verbose,
        crate::parallel::effective_jobs(params.jobs),
        compiler_cache.as_deref(),
    ).await?
} else {
    Vec::new()
};

Files changed

File	Change
`pipeline/library.rs`	Added `resolve_lib_deps()` (~50 lines)
`pipeline/mod.rs`	Re-export it
13 orchestrator files	Each ~~55→~~22 lines

Net: +593/−60 across 16 files. ~440 lines of duplicated code eliminated.

Orchestrators wired

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Configured library dependencies are now automatically resolved, downloaded, and compiled across supported ARM, ESP8266, AVR, and CH32V targets.
- Resolved libraries are included in compilation and linking, improving support for projects with external dependencies.
- Library ignore settings are honored during dependency resolution.
Bug Fixes
- Builds now correctly pass resolved library archives through the build pipeline instead of omitting them.
- Projects without configured dependencies continue to build without unnecessary dependency processing.

coderabbitai · 2026-08-10T00:20:07Z

📝 Walkthrough

Walkthrough

The PR adds shared resolve_lib_deps support and integrates configured library dependency resolution across ARM, MCU, and ESP8266 orchestrators. Builds now add dependency include paths and link resolved archives.

Changes

Library dependency integration

Layer / File(s)	Summary
Shared dependency resolution helper `crates/fbuild-build-engine/src/pipeline/library.rs`, `crates/fbuild-build-engine/src/pipeline/mod.rs`	Adds and re-exports `resolve_lib_deps`. The helper handles empty dependencies, selects the archiver, stages dependencies, updates include paths, and returns archives.
ARM orchestrator integration `crates/fbuild-build-arm/src//orchestrator.rs`, `crates/fbuild-build-arm/src/stm32/orchestrator/`	ARM orchestrators resolve `lib_deps` before final compiler creation, honor `lib_ignore`, update include paths, and pass archives to sequential builds.
MCU and ESP orchestrator integration `crates/fbuild-build-mcu/src/{avr,ch32v}/orchestrator.rs`, `crates/fbuild-build-esp/src/esp8266/orchestrator.rs`	AVR adopts the shared helper. CH32V and ESP8266 resolve dependencies before compilation and pass the resulting archives to the build pipeline.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Orchestrator
  participant TemporaryCompiler
  participant LibraryResolver
  participant SequentialBuild
  Orchestrator->>TemporaryCompiler: derive compiler flags
  Orchestrator->>LibraryResolver: resolve lib_deps and lib_ignore
  LibraryResolver-->>Orchestrator: include paths and archives
  Orchestrator->>SequentialBuild: compile and link with resolved archives

Possibly related PRs

FastLED/fbuild#1016: Extracted the build engine and established the related platform integration context.
FastLED/fbuild#1197: Also modified the STM32 Arduino mbed orchestrator.
FastLED/fbuild#1257: Added overlapping dependency-resolution and staged archive handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the refactor and its purpose: centralizing resolve_lib_deps() to remove duplicated orchestrator wiring.
Linked Issues check	✅ Passed	The listed orchestrators now resolve lib_deps before compiler creation, honor lib_ignore, add include paths, and pass archives to the build pipeline.
Out of Scope Changes check	✅ Passed	The changes remain within the linked issue scope and support the stated refactor without unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/1292-rollout-ensure-lib-deps

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…ib_deps wiring across 13 orchestrators Each orchestrator had a ~55-line copy-paste block that read lib_deps / lib_ignore from config, built a temp compiler for c/cxx flags, picked the archiver, called ensure_lib_deps(), extended include_dirs, and passed archives to run_sequential_build_with_libs. The only variation between them was the temp compiler constructor and how they accessed c/cpp flags. Extract the invariant body into pipeline::resolve_lib_deps() in the library module. Each orchestrator now creates a temp compiler (the one varying part), then delegates to resolve_lib_deps() for everything else. Fixes #1292 (roll-out of ensure_lib_deps across all remaining orchestrators, with the duplication refactored into a single site). Co-Authored-By: Claude <noreply@anthropic.com>

Matches the same annotation on the sibling ensure_lib_deps function. Co-Authored-By: Claude <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fbuild-build-arm/src/apollo3/orchestrator.rs`:
- Around line 275-302: Temporary compilers passed to resolve_lib_deps do not
apply the project’s build_unflags before their flags are read. In
crates/fbuild-build-arm/src/apollo3/orchestrator.rs:275-302,
crates/fbuild-build-arm/src/nrf52/orchestrator.rs:309-336,
crates/fbuild-build-arm/src/nxplpc/orchestrator.rs:318-345,
crates/fbuild-build-arm/src/rp2040/orchestrator.rs:339-366,
crates/fbuild-build-arm/src/silabs/orchestrator.rs:184-211,
crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs:140-167,
crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs:364-391, and
crates/fbuild-build-arm/src/teensy/orchestrator.rs:269-296, apply
ctx.build_unflags.clone() to each temp_compiler before calling Compiler::c_flags
and cpp_flags. Add a failing regression build using tempfile, real filesystem
behavior, a temporary project, and a real dependency fixture.

In `@crates/fbuild-build-engine/src/pipeline/library.rs`:
- Around line 398-437: Add behavioral tests for the public resolve_lib_deps
function using tempfile-backed real filesystem setup: cover empty lib_deps
returning no archives without unnecessary work, and an integration path that
confirms returned include directories are used during compilation and returned
archives during linking. Follow the existing test conventions and create the
failing tests before implementation changes, without replacing filesystem
behavior with mocks.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 350307a6-2474-4c15-8fcc-a4094ba2b599

📥 Commits

Reviewing files that changed from the base of the PR and between 3bb8f4d and c49f75e.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (15)

crates/fbuild-build-arm/src/apollo3/orchestrator.rs
crates/fbuild-build-arm/src/nrf52/orchestrator.rs
crates/fbuild-build-arm/src/nxplpc/orchestrator.rs
crates/fbuild-build-arm/src/renesas/orchestrator.rs
crates/fbuild-build-arm/src/rp2040/orchestrator.rs
crates/fbuild-build-arm/src/sam/orchestrator.rs
crates/fbuild-build-arm/src/silabs/orchestrator.rs
crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs
crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs
crates/fbuild-build-arm/src/teensy/orchestrator.rs
crates/fbuild-build-engine/src/pipeline/library.rs
crates/fbuild-build-engine/src/pipeline/mod.rs
crates/fbuild-build-esp/src/esp8266/orchestrator.rs
crates/fbuild-build-mcu/src/avr/orchestrator.rs
crates/fbuild-build-mcu/src/ch32v/orchestrator.rs

coderabbitai · 2026-08-10T01:08:37Z

+            let temp_compiler = ArmCompiler::new(
+                toolchain.get_gcc_path(),
+                toolchain.get_gxx_path(),
+                &ctx.board.mcu,
+                &ctx.board.f_cpu,
+                defines.clone(),
+                include_dirs.clone(),
+                augmented_config.clone(),
+                params.profile,
+                params.verbose,
+            );
+            pipeline::resolve_lib_deps(
+                &lib_deps,
+                &lib_ignore,
+                &params.project_dir,
+                &ctx.build_dir,
+                &toolchain.get_gcc_path(),
+                &toolchain.get_gxx_path(),
+                &toolchain.get_ar_path(),
+                &toolchain.get_gcc_ar_path(),
+                &crate::compiler::Compiler::c_flags(&temp_compiler),
+                &crate::compiler::Compiler::cpp_flags(&temp_compiler),
+                &mut include_dirs,
+                params.verbose,
+                crate::parallel::effective_jobs(params.jobs),
+                None,
+            )
+            .await?


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Apply build_unflags to every temporary compiler.

Each final compiler applies with_build_unflags, but each temporary compiler used by resolve_lib_deps does not. A configured build_unflags value can therefore make a dependency archive compile with flags that the project build removes. This can cause dependency compilation failures or incompatible compile settings.

Apply the same unflag configuration before reading temporary compiler flags. Add a failing regression build that uses a temporary project and a real dependency fixture.

crates/fbuild-build-arm/src/apollo3/orchestrator.rs#L275-L302: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/nrf52/orchestrator.rs#L309-L336: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391: apply ctx.build_unflags.clone() to temp_compiler.

crates/fbuild-build-arm/src/teensy/orchestrator.rs#L269-L296: apply ctx.build_unflags.clone() to temp_compiler.

As per coding guidelines, “Follow TDD” and “Use tempfile and real filesystem behavior for filesystem tests rather than mocks.”

📍 Affects 8 files

crates/fbuild-build-arm/src/apollo3/orchestrator.rs#L275-L302 (this comment)

crates/fbuild-build-arm/src/nrf52/orchestrator.rs#L309-L336

crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345

crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366

crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211

crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167

crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391

crates/fbuild-build-arm/src/teensy/orchestrator.rs#L269-L296

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build-arm/src/apollo3/orchestrator.rs` around lines 275 - 302, Temporary compilers passed to resolve_lib_deps do not apply the project’s build_unflags before their flags are read. In crates/fbuild-build-arm/src/apollo3/orchestrator.rs:275-302, crates/fbuild-build-arm/src/nrf52/orchestrator.rs:309-336, crates/fbuild-build-arm/src/nxplpc/orchestrator.rs:318-345, crates/fbuild-build-arm/src/rp2040/orchestrator.rs:339-366, crates/fbuild-build-arm/src/silabs/orchestrator.rs:184-211, crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs:140-167, crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs:364-391, and crates/fbuild-build-arm/src/teensy/orchestrator.rs:269-296, apply ctx.build_unflags.clone() to each temp_compiler before calling Compiler::c_flags and cpp_flags. Add a failing regression build using tempfile, real filesystem behavior, a temporary project, and a real dependency fixture.

Source: Coding guidelines

coderabbitai · 2026-08-10T01:08:37Z

+pub async fn resolve_lib_deps(
+    lib_deps: &[String],
+    lib_ignore: &[String],
+    project_dir: &Path,
+    build_dir: &Path,
+    gcc_path: &Path,
+    gxx_path: &Path,
+    ar_path: &Path,
+    gcc_ar_path: &Path,
+    c_flags: &[String],
+    cpp_flags: &[String],
+    include_dirs: &mut Vec<PathBuf>,
+    verbose: bool,
+    jobs: usize,
+    compiler_cache: Option<&Path>,
+) -> Result<Vec<PathBuf>> {
+    if lib_deps.is_empty() {
+        return Ok(Vec::new());
+    }
+    let dep_lib_ar_path = pick_archiver(ar_path, gcc_ar_path, c_flags, cpp_flags);
+    let libs_dir = build_dir.join("libs");
+    let (lib_include_dirs, archives) = ensure_lib_deps(
+        lib_deps,
+        lib_ignore,
+        gcc_path,
+        gxx_path,
+        dep_lib_ar_path,
+        c_flags,
+        cpp_flags,
+        include_dirs,
+        project_dir,
+        &libs_dir,
+        verbose,
+        jobs,
+        compiler_cache,
+    )
+    .await?;
+    include_dirs.extend(lib_include_dirs);
+    Ok(archives)
+}


📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add behavioral tests for resolve_lib_deps.

This public helper changes the compiler and linker input contract. This change adds no test for that contract. Add a tempfile-backed test for the empty dependency path. Add an integration test that verifies include directories reach compilation and archives reach linking.

As per coding guidelines, “Follow TDD: write failing tests first” and “Use tempfile and real filesystem behavior for filesystem tests rather than mocks.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build-engine/src/pipeline/library.rs` around lines 398 - 437, Add behavioral tests for the public resolve_lib_deps function using tempfile-backed real filesystem setup: cover empty lib_deps returning no archives without unnecessary work, and an integration path that confirms returned include directories are used during compilation and returned archives during linking. Follow the existing test conventions and create the failing tests before implementation changes, without replacing filesystem behavior with mocks.

Source: Coding guidelines

fastled-project-sync Bot added this to FastLED Tracker Aug 10, 2026

zackees force-pushed the fix/1292-rollout-ensure-lib-deps branch from aca222a to bc7537a Compare August 10, 2026 00:27

zackees and others added 2 commits August 9, 2026 18:04

fix(build-engine): allow too_many_arguments on resolve_lib_deps

c49f75e

Matches the same annotation on the sibling ensure_lib_deps function. Co-Authored-By: Claude <noreply@anthropic.com>

zackees force-pushed the fix/1292-rollout-ensure-lib-deps branch from 9d5b8e8 to c49f75e Compare August 10, 2026 01:04

coderabbitai Bot requested changes Aug 10, 2026

View reviewed changes

zackees merged commit 8e0cf68 into main Aug 10, 2026
91 of 92 checks passed

fastled-project-sync Bot moved this to Triage in FastLED Tracker Aug 10, 2026

Navigation Menu

Uh oh!

refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators - #1294

refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators#1294
zackees merged 2 commits into
mainfrom
fix/1292-rollout-ensure-lib-deps

zackees commented Aug 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Uh oh!

coderabbitai Bot Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

zackees commented Aug 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Before

After

Files changed

Orchestrators wired

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

zackees commented Aug 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading